-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: track event delivery stats #3974
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3974 +/- ##
==========================================
+ Coverage 71.94% 71.98% +0.03%
==========================================
Files 373 374 +1
Lines 54933 54966 +33
==========================================
+ Hits 39522 39565 +43
+ Misses 13095 13088 -7
+ Partials 2316 2313 -3
☔ View full report in Codecov by Sentry. |
if metric.StatusDetail.Status == "aborted" { | ||
metricName = EventsAbortedMetricName | ||
} | ||
stats.Default.NewTaggedStat(metricName, stats.CountType, tags).Count(int(metric.StatusDetail.Count)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use DI for stats, and convert the function to a method and add tests cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure leo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hint: You can use memstats to assist in your testing.
Example:
require.EqualValues(t, statsStore.Get("pgnotifier.publish", stats.Tags{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lvrach I have added test cases and changed it to method. Let me know if i got you correct
Remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some cleanup is needed on the default reporter
enterprise/reporting/reporting.go
Outdated
@@ -108,6 +110,8 @@ func NewDefaultReporter(ctx context.Context, log logger.Logger, configSubscriber | |||
maxOpenConnections: maxOpenConnections, | |||
maxConcurrentRequests: maxConcurrentRequests, | |||
dbQueryTimeout: dbQueryTimeout, | |||
statsReporter: NewEventStatsReporter(configSubscriber, stats), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statsReporter: NewEventStatsReporter(configSubscriber, stats), |
enterprise/reporting/reporting.go
Outdated
@@ -69,9 +69,11 @@ type DefaultReporter struct { | |||
getMinReportedAtQueryTime stats.Measurement | |||
getReportsQueryTime stats.Measurement | |||
requestLatency stats.Measurement | |||
statsReporter *EventStatsReporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statsReporter *EventStatsReporter |
…udder-server into feat.unifiedPipelineMetrics
@gane5hvarma is the Do not merge label still necessary? |
@atzoum thanks for pointing out. Removing the label |
Description
Completes OBS-148
The motivation for this pr is to have a single prom metrics the captures successful and aborted event delivery count across all stages. The metrics are used to calculate aborted percentage in mimir and send a notification to customers. This change is needed for the customer configurable alerts project
Linear Ticket
https://linear.app/rudderstack/project/%5Bp0p3%5D-health-customer-config-alerts-and-settings-phase-1-78dc57e675f8
https://linear.app/rudderstack/issue/OBS-148/single-metric-to-capture-failures-at-all-stages-of-the-pipeline-and
Security